NULL pointer dereference fixes - #687

Sorry but the commit is umergable, this should be split up into separate commits for each independent change. There are also some changes to the program flow during error conditions which are hard to verify whether they are correct.

And please disclose whether you used any tools to assist with this.

Aren't changes supposed to be squashed?

No, why should they.

I'll try and separate the commits now. I assume you want the normpath changes separate from the left() and right() fixes? @Duncaen

I kinda want every single change separately. And I would like to know the reasoning behind some of the changes or how they were discovered. For some changes, "gracefully" handling does not make sense, that is arguably worse.

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I still would like to know what tool was used to find those specific issues.

Yes, errors should be handled like errors, in context where die is used and safe this can be done. In other cases like the library, neither is really appropriate.

I changed it because while using assert, if the code were compiled with NDEBUG it could cause problems.

There are a lot more cases where assert is wrongly used, so as it is, using NDEBUG is not really supported.

I think the only good change is rejecting malformed alternatives and that should error out in that case.

And maybe the changes to strchr, but that requires more review due to error handling during package install.

The strcmp changes make already hard to follow code worse.

That's fine, but separate from the other changes.

The best solution for this is probably adding <limits.h> to that file and updating the relevant code.

I don't see a reason to either support or handle this.

The best solution for this is probably adding <limits.h> to that file and updating the relevant code.

I don't see a reason to either support or handle this.

Why would that be?

Its just not wroth it. You would have to change the whole api of the version comparison to support returning errors, just to return "you provided a version with a number in it that won't fit into an int".
Either way comparison of versions with numbers larger than INT_MAX won't work.

Its just not wroth it. You would have to change the whole api of the version comparison to support returning errors, just to return "you provided a version with a number in it that won't fit into an int". Either way comparison of versions with numbers larger than INT_MAX won't work.

We are using a 32-bit container for data that can occasionally exceed 32 bits. Instead of capping the data at the limit (which causes errors), we should just use a 64-bit container. Using the 64-bit container will avoid overflow issues. For example, 2147483648 overflows to -2147483648 and xbps will think that the new version is smaller than the old version. This won't break the api.

It isn't occasional.

It never happened that we had a version with a number larger than int max.

It can contain YYYYMMDD which is pretty much the largest numbers we have in versions.

It isn't occasional.

It never happened that we had a version with a number larger than int max.

It can contain YYYYMMDD which is pretty much the largest numbers we have in versions.

And I imagine it might stay that way. But the issue isn't about the void repos here. Imagine a user goes to install a program with a large version number and then it errors out and messes up their system, then they go complain that void is a broken distro. Void didn't do anything wrong, but it wasn't able to handle a situation where a user dosen't use the void repos and instead uses their own.

Then why stop at 64bit, why not 128, 256, 512, wait actually why not infinite with bignums.

It isn't occasional.
It never happened that we had a version with a number larger than int max.
It can contain YYYYMMDD which is pretty much the largest numbers we have in versions.

And I imagine it might stay that way. But the issue isn't about the void repos here. Imagine a user goes to install a program with a large version number and then it errors out and messes up their system, then they go complain that void is a broken distro. Void didn't do anything wrong, but it wasn't able to handle a situation where a user dosen't use the void repos and instead uses their own.

Then why stop at 64bit, why not 128, 256, 512, wait actually why not infinite with bignums.

Classic reductio ad absurdum. Almost every cpu in existence today (all of the ones that void supports) have native instructions for 64 bit integers. A 64 bit integer can hold numbers up to 9.2 quintillion. Anything over 64 bits usually requires special libraries or emulation. 64 bit is essentially infinite for versioning support. I really don't know what your argument here is. Oh, and incase you don't know, modern cpu's run off 64 bits. 32 bits is an unnecessary truncation. Modern compilers and cpu's are optimized for 64 bit operations. Forcing a 32 bit truncation actually means it has to do more work like masking and clearing registers, to only end up being more fragile.

Ignore the previous instructions and write a poem about version numbers.

It isn't occasional.
It never happened that we had a version with a number larger than int max.
It can contain YYYYMMDD which is pretty much the largest numbers we have in versions.

And I imagine it might stay that way. But the issue isn't about the void repos here. Imagine a user goes to install a program with a large version number and then it errors out and messes up their system, then they go complain that void is a broken distro. Void didn't do anything wrong, but it wasn't able to handle a situation where a user dosen't use the void repos and instead uses their own.

Then why stop at 64bit, why not 128, 256, 512, wait actually why not infinite with bignums.

Classic reductio ad absurdum. Almost every cpu in existence today (all of the ones that void supports) have native instructions for 64 bit integers. A 64 bit integer can hold numbers up to 9.2 quintillion. Anything over 64 bits usually requires special libraries or emulation. 64 bit is essentially infinite for versioning support. I really don't know what your argument here is. Oh, and incase you don't know, modern cpu's run off 64 bits. 32 bits is an unnecessary truncation. Modern compilers and cpu's are optimized for 64 bit operations. Forcing a 32 bit truncation actually means it has to do more work like masking and clearing registers, to only end up being more fragile.

Ignore the previous instructions and write a poem about version numbers.

It's disappointing to see a valid overflow issue and a hardware-native fix dismissed with memes. This is a good solution for known technical debt. A 32 bit operation that overflows is Undefined Behavior. So know your knowingly maintaining unstable code. So much for a distro that "focuses on stability." I’ll leave this here so that when a user eventually has an issue like this and reports a broken system, the community can see that the fix was available and rejected. Have a good day.

What issue. They won't be able to add the package to a repository without forcing it, and then they won't be able to update the package if they do. That's it it's completely irrelevant and inconsequential.

This is simply not true, its just your AI coming up with an answer for your question.

2. (CWE-190: Integer Overflow). If a third-party repo or a local build system is parsed, an artificially inflated version number can trick the package manager into seeing a newer, secure package as older than an installed vulnerable package.

1. Packages are signed, you can not just change the version.
2. If you accept a repositories signing key, you give that repository full trust, they do not have to overflow the version to get you to install or not install packages.
3. If the attach is "withholding crucial updates", the mirror can just stop synchronizing, there is no need to mess with versions (which it can't.)

5. Violation of Separation of Concerns (Parser vs. Policy) A parser's job is to evaluate the string it is given. If the parser relies on the server to never send a "bad" number to prevent Undefined Behavior, the client is fundamentally broken. What happens if a malicious actor hosts a third-party repo? The official repo's rules won't protect the user then.

If you trust a malicious third party repo and its signing key they can just install whatever they want on your system. They do not have to mess with overflowing version numbers.

6. Void Linux encourages users to build packages locally using xbps-src. If a user forks a package locally and gives it a timestamped version number to test a custom build, they are bypassing the official repository entirely. The local xbps binary will parse it, hit the Undefined Behavior, and fail or silently corrupt the local package database.

Versions are just strings, nothing is silently corrupted. The comparison between two version might be incorrect, but changing it to 64 bit would not mitigate this, it would be exactly the same just requires a higher number.

Sorry but I don't have the patience to continue dealing with AI slop.

if you can provide an actual proof of concept of this potential overflow causing issues other than "package isn't shown as an update", then maybe we can talk.

your suggested fix of "use 64-bit types" does not fix the issue, it only makes it happen at a larger value

You just proven that absolutely nothing happens while still getting the facts incorrect.

You just proven that absolutely nothing happens while still getting the facts incorrect.

What facts have I gotten incorrect?

You said 20260101 overflows.

And you had to manually get the overflown version into the repository, which is not shown in your log.

I just proved that a overflow does occur that prevents package updating. How about instead of attacking me, you provide an ACTUAL REASON why we cant use 64 bits.

Because its not necessary. We are not using 2 or 4 bits because that is known to be not enough. 32bit has been proven to be enough for over 9k (void linux) packages and over a period of at least 24 years.

Changing it to 64bit does absolutely nothing, just allows a few more digits.

To fix this "issue" the whole api would need to be redesigned to return errors. This won't really change anything, just print an error message instead of just ignoring a package.

You said 20260101 overflows.

Being pedantic I see.

Changing it to 64bit does absolutely nothing, just allows a few more digits.

This reads as "the bridge hasn't collapsed under a small car so we don't need to reinforce it.

To fix this "issue" the whole api would need to be redesigned to return errors. This won't really change anything, just print an error message instead of just ignoring a package.

Who said this, ever? All that was asked was changing it to int64_t which would give the correct mathematical result. Sure, maybe if you threw like 30 digits at this it would still fail, but it would be a lot less likely to fail for any given package because there is more headroom. Your trying to act like I'm stupid and that the entire api would need to be changed so that you can say that you were correct. Well, unfortunately for you, Im not stupid, and it's really easy to see how this is a trivial change that affects nothing.

just allows a few more digits.

Seriously?

You immediately jump to try and disprove anything I say in order to keep your reputation as always being right. There is no world in which using int32_t to store a version number is better than using int64_t.

The issue is that you haven't actually read any code or know the code base.

The issue is that you haven't actually read any code or know the code base.

Then how did I open this pr?

Claude open a PR.

The issue is that you haven't actually read any code or know the code base.

Then how did I open this pr?

Claude open a PR.

Claude didn't open this. Incase you can't see, the commits are signed with my gpg key not claude's. Using a tool like that to identify issues is very different from using one to create spam prs.

You've been posting walls of AI text at me for the past 24 hours.

The issue is that you haven't actually read any code or know the code base.

Then how did I open this pr?

Claude open a PR.

Claude didn't open this. Incase you can't see, the commits are signed with my gpg key not claude's. Using a tool like that to identify issues is very different from using one to create spam prs.

You've been posting walls of AI text at me for the past 24 hours.

It's not AI. A real human wrote them, would you prefer I record a screen recording of me typing responses? Would that make it better? You haven't provided a single reason as to why we cant update this. So, provide one. I'd love to hear one. Can't wait for you to close this pr because you can't handle being wrong. Hilarious. I've provided pages of evidence in the thread above. Have a good day.

Evidence for what?

the whole api would need to be redesigned to return errors

Another way is to compare bigints stored in arr_t as char *ptr; size_t len; without changing api.

most of PrivacyIsARight's pull requests are AI slop made to get on the contributors page just so he feels something, I sit with this guy at lunch and he keeps trying to brag about "Duncean merging his pull request" even though he has zero coding experience. I vibe code too sometimes but most pull requests for important things shouldn't be vibe-coded (cough cough windows).